Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ttonev/hr portal #3

Merged
merged 22 commits into from
Apr 1, 2025
Merged

Ttonev/hr portal #3

merged 22 commits into from
Apr 1, 2025

Conversation

tishko0
Copy link
Contributor

@tishko0 tishko0 commented Mar 20, 2025

No description provided.

@tishko0 tishko0 requested a review from dkamburov March 20, 2025 10:02
@dkamburov dkamburov requested a review from mddragnev March 20, 2025 12:20
@MayaKirova
Copy link

MayaKirova commented Mar 24, 2025

  • There are some minor difference between the angular toolbar and this one.

    Here's a comparison, angular on the right, wc on the left.
    image

    Notice that:

    • Title "HR Portal" is not aligned to the left in the wc one.
    • Button text in the toolbar is darker in the Angular one, compared to the wc one.
  • Also after sorting a column, in angular a new button for clearing sort shows. In wc it does not show:

image

  • If we compare both the angular and wc sample with the figma design:

    • Title in design is "Actions", while in the sample it is "HR Portal"
    • Column "Name" is initially sorted in the design, but not in the samples.

    Either the samples or the design should be updated so that there are no differences.

  • Noticed that there are also some console.logs left over. Those should be removed:
    image

@MayaKirova MayaKirova added ✅ status: verified Applies to PRs that have passed manual verification 🛠️ status: in-development Issues and PRs with active development on them and removed ✅ status: verified Applies to PRs that have passed manual verification labels Mar 24, 2025
@dkamburov dkamburov added ❌ status: awaiting-test PRs awaiting manual verification and removed 🛠️ status: in-development Issues and PRs with active development on them labels Mar 26, 2025
@tishko0
Copy link
Contributor Author

tishko0 commented Mar 26, 2025

  • There are some minor difference between the angular toolbar and this one.
    Here's a comparison, angular on the right, wc on the left.
    image
    Notice that:

    • Title "HR Portal" is not aligned to the left in the wc one.
    • Button text in the toolbar is darker in the Angular one, compared to the wc one.
  • Also after sorting a column, in angular a new button for clearing sort shows. In wc it does not show:

image

  • If we compare both the angular and wc sample with the figma design:

    • Title in design is "Actions", while in the sample it is "HR Portal"
    • Column "Name" is initially sorted in the design, but not in the samples.

    Either the samples or the design should be updated so that there are no differences.

  • Noticed that there are also some console.logs left over. Those should be removed:
    image

Added the sorting button and changed the title to actions. Regarding text color, I have not set any formatting to it in both samples, so i guess it comes with difference in themes between WC in Angular. We can comment further if this needs to be changed. Regarding the toolbar title alignment, it does not align on the right side for me. Maybe someone else can test it too?

@tishko0 tishko0 closed this Mar 26, 2025
@tishko0 tishko0 reopened this Mar 26, 2025
@tishko0 tishko0 requested a review from mddragnev March 26, 2025 10:29
Copy link
Member

@mddragnev mddragnev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the comments and resolve conflicts

@MayaKirova
Copy link

MayaKirova commented Mar 26, 2025

  • There are some minor difference between the angular toolbar and this one.
    Here's a comparison, angular on the right, wc on the left.
    image
    Notice that:

    • Title "HR Portal" is not aligned to the left in the wc one.
    • Button text in the toolbar is darker in the Angular one, compared to the wc one.
  • Also after sorting a column, in angular a new button for clearing sort shows. In wc it does not show:

image

  • If we compare both the angular and wc sample with the figma design:

    • Title in design is "Actions", while in the sample it is "HR Portal"
    • Column "Name" is initially sorted in the design, but not in the samples.

    Either the samples or the design should be updated so that there are no differences.

  • Noticed that there are also some console.logs left over. Those should be removed:
    image

Added the sorting button and changed the title to actions. Regarding text color, I have not set any formatting to it in both samples, so i guess it comes with difference in themes between WC in Angular. We can comment further if this needs to be changed. Regarding the toolbar title alignment, it does not align on the right side for me. Maybe someone else can test it too?

@tishko0

Then the text has the same styles as in angular:

image

So it seems to be some issue with the sample or the configuration in lit with the theming.

  • Regarding the text of the title, I assume the design is outdated and the one in angular is the correct one. @dkamburov probably has more information.

  • Another issue I noticed with the latest changes is that the sample does not flex to the full 100% width:
    image
    It's probably missing a flex-grow or something to grow the content to the page size.

  • Also, the "Clear Sort" button is there, but for some reason the icon is outside of it and it doesn't look good when you hover it:
    image

It should be inside, like in angular:
image

@MayaKirova MayaKirova added 🛠️ status: in-development Issues and PRs with active development on them and removed ❌ status: awaiting-test PRs awaiting manual verification labels Mar 26, 2025
@tishko0 tishko0 requested a review from mddragnev March 27, 2025 12:55
@MayaKirova MayaKirova added 💥 status: in-test PRs currently being tested and removed 🛠️ status: in-development Issues and PRs with active development on them labels Mar 28, 2025
@MayaKirova MayaKirova added ✅ status: verified Applies to PRs that have passed manual verification and removed 💥 status: in-test PRs currently being tested labels Mar 28, 2025
@dkamburov dkamburov merged commit f1eafaa into vnext Apr 1, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ status: verified Applies to PRs that have passed manual verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants